Change the mapper output directory from $TMP/shards to $TMP/map_output #3960
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@gitlw you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Thanks for the detailed context via the PR description. Going through, the PR seems to fix the issue as a workaround as mentioned. In terms of the actual big, it's still there if someone decided in the future to do something similar - so I would suggest adding a comment that the root issue still exists/but you were unable to reproduce.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @manishrjain)
|
I have a question. Wouldn't it make more sense to run |
|
@mangalaman93 I think fsync would help with other types of problems. |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gitlw and @manishrjain)
dgraph/cmd/bulk/mapper.go, line 92 at r1 (raw file):
filename := filepath.Join( m.opt.TmpDir, "map_output",
perhaps make this as constant.
dgraph/cmd/bulk/merge_shards.go, line 30 at r1 (raw file):
func mergeMapShardsIntoReduceShards(opt options) { mapShards := shardDirs(opt.TmpDir + "/map_output")
use filepaht.Join to make this logic more robust
dgraph/cmd/bulk/reduce.go, line 48 at r1 (raw file):
func (r *reducer) run() error { dirs := shardDirs(r.opt.TmpDir + "/shards")
also use filepath.Join here
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @martinmr)
dgraph/cmd/bulk/mapper.go, line 92 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
perhaps make this as constant.
Done.
dgraph/cmd/bulk/merge_shards.go, line 30 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
use filepaht.Join to make this logic more robust
Done.
dgraph/cmd/bulk/reduce.go, line 48 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
also use filepath.Join here
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @martinmr)
In #3959 , bulk loader crashes when trying to move a directory into itself with a new name
/dgraph/tmp/shards/shard_0
/dgraph/tmp/shards/shard_0/shard_0
The bulk loader logic is
the mapper produce output as
.../tmp/shards/000
.../tmp/shards/001
read the list of shards under .../tmp/shards/
create the reducer shards as
.../tmp/shards/shard_0
.../tmp/shards/shard_1
move the list read in step 2 into the reducer shards created in step 3
Though I cannot reproduce the problem, but it seems creating of the reducer shard directory .../tmp/shards/shard_0 and listing all the mapper shards in step 2 are re-ordered. Something similar is mentioned in etcd-io/etcd#6368
This PR avoids such possibilities by putting the mapper output into an independent directory
../tmp/map_output, so that the program works correctly even if the reordering happens.
This change is